Skip to content

Conversation

@lzsiga
Copy link

@lzsiga lzsiga commented Aug 30, 2021

On 64-bit big-endian platforms pointers to 8-bytes field cannot be
trivially converted into pointers to 4-bytes field.

Explained in details here: https://bugs.php.net/bug.php?id=73002

This affects strings' length-fields, booleans, and integer values for Oracle client 10 (and older).

trivially converted into pointers to 4-bytes field.
Explained in details here: https://bugs.php.net/bug.php?id=73002
@cmb69
Copy link
Member

cmb69 commented Oct 26, 2021

Thank you for the PR! As I understand it, this is a real bug fix, and as such might better target PHP-7.4 or PHP-8.0. And having a test case which shows the erroneous behavior would be welcome (even if the test would not fail on CI, or even not executed there).

Anyhow, @cjbj, could you have a look at this PR?

unnamed PL/SQL-block, assign value to bind-variable (string type)
#define ZendLongPtr2OciPhpsizedIntPtr(zendlongptr) \
((oci_phpsized_int *)((char *)(zendlongptr)+(sizeof(zend_long)-sizeof(oci_phpsized_int))))
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this does not looks like a proper fix

Moving the pointer is not the proper way when size differ, especially as other bytes are not initialized
Should use a temp storage of the proper size.

Copy link
Author

@lzsiga lzsiga Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, using uint64_t * (or ub8 * in Oracle terms) pointers instead of uint32_t * (or ub4 *) pointers is not correct. This problematic behaviour wasn't created by my patch, it already existed; the patch only made it visible, as in big-endian platform it is different:

little-endian: ub4* shortintptr= (ub4 *)&ub8variable;
big-endian:    ub4* shortintptr= (ub4 *)((char *)&ub8variable+4);

Refactoring the code using temporary variables would be possible (they should put into struct php_oci_bind), but would require quite a bit of work (cf https://bugs.php.net/bug.php?id=73002#1473330938 )

@lzsiga
Copy link
Author

lzsiga commented Oct 28, 2021

Hi @cmb69
Here's the first draft of a simple test: https://github.com/lzsiga/php-src/blob/master/ext/oci8/tests/bigendian64_1.phpt

@lzsiga lzsiga requested a review from remicollet December 4, 2021 09:17
@lzsiga
Copy link
Author

lzsiga commented May 22, 2022

Thank you guys for all your efforts. Best wishes.

@lzsiga lzsiga closed this May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants